fix: separate maintenance from processOne (#122)#134
Conversation
processOne was mixing claim/execute with maintenance (releaseExpiredLeases, auto-purge). The fire-and-forget purge broke the "false = idle" contract. - Simplify processOne to pure claim → execute → return true/false - Move releaseExpiredLeases and auto-purge into runIdleMaintenance - Add onIdle callback to worker, called when processOne returns false - processUntilIdle runs maintenance only when actually idle (not maxRuns) - stop() awaits in-flight idle maintenance via unified cycle promise - Wrap runIdleMaintenance in try/catch to prevent worker loop hangs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughアイドル時に保守タスクを分離して導入。runIdleMaintenanceをDurablyStateに追加し、期限切れリース解放と古いターミナルランの削除をアイドル周期で実行するように変更。クレームパスの即時自動削除を削除し、workerにonIdleフックを追加。 Changes
Sequence DiagramsequenceDiagram
participant App as アプリケーション
participant Worker as Worker
participant Durably as Durably
participant State as State\n(runIdleMaintenance)
participant Store as ストレージ
App->>Worker: pollRuns開始
loop ポーリングサイクル
Worker->>Durably: processOne()
Durably->>Store: claimNextPendingRun / クレーム試行
alt ランあり
Durably-->>Worker: true (処理実行)
Durably->>Store: ラン処理・更新
else ランなし(アイドル)
Durably-->>Worker: false (アイドル)
alt onIdleが設定されている
Worker->>State: onIdle() を呼び出す
State->>State: runIdleMaintenance 実行
State->>Store: 期限切れリース解放
State->>Store: 古いターミナルラン削除(retainRunsMs適用)
State-->>Worker: 保守完了
end
end
end
Worker-->>App: 停止/完了
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/durably/src/durably.ts (1)
1045-1046:lastPurgeAtがローカル変数に変更された点について
lastPurgeAtがDurablyStateから削除され、createDurablyのローカル変数になりました。これは実装詳細を隠蔽する良い変更ですが、テストコメント(purge.shared.ts の 247-248 行、261-263 行)でlastPurgeAtへの言及が残っています。テストコメントは内部状態を説明するためのものなので問題ありませんが、将来のメンテナ向けにコメントが内部実装を参照していることを認識しておいてください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/src/durably.ts` around lines 1045 - 1046, Tests still mention the internal variable lastPurgeAt which was removed from DurablyState and made a local variable inside createDurably; update the test comments in purge.shared.ts (around the existing references) to stop referring to lastPurgeAt directly — either remove the internal-variable mention or replace it with a comment that the value is now held internally within createDurably (DurablyState -> createDurably, lastPurgeAt) so future readers aren’t misled by implementation details.packages/durably/src/worker.ts (1)
36-48:onIdle()での例外がポーリングループをクラッシュさせる可能性
onIdle()が例外をスローした場合、poll()内で捕捉されず、ポーリングループが停止してしまいます。durably.ts側のrunIdleMaintenanceは try/catch でラップされていますが、将来的に異なるonIdleコールバックを渡す可能性を考慮すると、ここで防御的に処理することを検討してください。🛡️ 防御的エラーハンドリングの提案
const cycle = (async () => { const didProcess = await processOne({ workerId: activeWorkerId }) if (!didProcess && onIdle && running) { - await onIdle() + try { + await onIdle() + } catch { + // onIdle errors are non-fatal; allow polling to continue + } } })()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/src/worker.ts` around lines 36 - 48, The polling cycle currently awaits onIdle() without handling exceptions, so any error thrown by the onIdle callback can crash the poll loop; inside the anonymous async cycle (the variable cycle) that calls processOne({ workerId: activeWorkerId }) and then awaits onIdle(), wrap the onIdle() invocation in a try/catch (or otherwise guard the await) so exceptions are caught and handled (e.g., log via the same logger or swallow) and do not escape the cycle; keep the existing inFlight assignment and finally block behavior unchanged so inFlight is still cleared on completion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/durably/src/durably.ts`:
- Around line 1045-1046: Tests still mention the internal variable lastPurgeAt
which was removed from DurablyState and made a local variable inside
createDurably; update the test comments in purge.shared.ts (around the existing
references) to stop referring to lastPurgeAt directly — either remove the
internal-variable mention or replace it with a comment that the value is now
held internally within createDurably (DurablyState -> createDurably,
lastPurgeAt) so future readers aren’t misled by implementation details.
In `@packages/durably/src/worker.ts`:
- Around line 36-48: The polling cycle currently awaits onIdle() without
handling exceptions, so any error thrown by the onIdle callback can crash the
poll loop; inside the anonymous async cycle (the variable cycle) that calls
processOne({ workerId: activeWorkerId }) and then awaits onIdle(), wrap the
onIdle() invocation in a try/catch (or otherwise guard the await) so exceptions
are caught and handled (e.g., log via the same logger or swallow) and do not
escape the cycle; keep the existing inFlight assignment and finally block
behavior unchanged so inFlight is still cleared on completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 594dcd07-6de1-41c8-ab4e-cbc5f55d7f15
📒 Files selected for processing (3)
packages/durably/src/durably.tspackages/durably/src/worker.tspackages/durably/tests/shared/purge.shared.ts
- Verify processUntilIdle({ maxRuns }) does NOT run maintenance when
backlog remains (only runs when actually idle)
- Verify stop() awaits in-flight idle maintenance before resolving
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add defensive try/catch for onIdle() in worker poll loop to prevent crashes from unexpected callback errors - Remove internal variable name references from test comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/durably/tests/shared/purge.shared.ts (1)
271-309: テストロジックは正しいが、d.stop()の呼び出しが欠落している可能性あり。テストロジック自体は正確です:
processUntilIdleはmaxRunsに達した場合はreachedIdleがfalseのままでメンテナンスを実行せず、真にアイドル状態になった時のみメンテナンスが実行されることを検証しています。ただし、Line 308で
d.db.destroy()のみ呼び出し、d.stop()を呼び出していません。このテストではd.start()を呼び出していないためワーカーポーリングは起動していませんが、PRの目的によるとstop()は「進行中のアイドルメンテナンスを待機する」ようになっています。processUntilIdle()は内部でメンテナンスを待機するため問題ない可能性がありますが、一貫性のためにd.stop()を追加することを検討してください。♻️ 一貫性のための修正案
// Now drain fully (reaches idle) — maintenance runs and purges await d.processUntilIdle() expect(await d.getRun(run1.id)).toBeNull() + await d.stop() await d.db.destroy() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/tests/shared/purge.shared.ts` around lines 271 - 309, The test omits shutting down the Durably instance: add an explicit await d.stop() before calling d.db.destroy() so the worker is cleanly stopped and any in-flight/idle maintenance is awaited; reference the Durably instance methods processUntilIdle, stop, and the existing d.db.destroy() call and insert the await d.stop() immediately prior to d.db.destroy().packages/durably/tests/shared/worker.shared.ts (2)
94-96: 未使用のイベントリスナー
run:leasedイベントリスナーが登録されていますが、このテストではジョブがトリガーされないため、このイベントは発火しません。コメントに「worker to process something」とありますが、実際には何も処理されません。このリスナーは削除するか、テストの意図に沿ったイベント(例:
worker:errorでメンテナンスエラーを監視)に変更することを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/tests/shared/worker.shared.ts` around lines 94 - 96, The test registers an unused event listener d.on('run:leased') which never fires; remove this noop listener or replace it with a meaningful one that matches the test intent (for example d.on('worker:error') to assert maintenance or error conditions). Locate the registration of d.on('run:leased') in the test (symbol: d.on) and either delete that line or change the event name and accompanying assertion to watch for the appropriate event (e.g., 'worker:error') so the test actually observes and asserts on emitted events.
81-110: テストアサーションが常に成功する(トートロジー)
maintenanceCompleted = trueをawait d.stop()の直後に設定し、その後expect(maintenanceCompleted).toBe(true)でアサートしていますが、これは逐次実行されるため常に成功します。このアサーションはメンテナンスが実際に待機されたことを検証していません。テストの価値は
stop()がハングしないことを確認する点にありますが、アサーションが誤解を招く形になっています。♻️ より意味のあるテストへの改善案
it('stop() awaits in-flight idle maintenance before resolving', async () => { - let maintenanceCompleted = false + let maintenanceStarted = false + let stopResolved = false // Use retainRuns to ensure runIdleMaintenance does real work const d = createDurably({ dialect: createDialect(), pollingIntervalMs: 50, retainRuns: '30d', }) await d.migrate() - // Listen for the idle-maintenance cycle completing via worker:error - // or simply track that stop() doesn't resolve before maintenance - d.on('run:leased', () => { - // noop — just need the worker to process something - }) + // Track idle maintenance via worker events if available + d.on('worker:error', () => { + // Maintenance errors would be emitted here + }) d.start() // Let the worker go through at least one idle cycle // (processOne returns false → onIdle runs releaseExpiredLeases) await new Promise((r) => setTimeout(r, 150)) + maintenanceStarted = true // stop() should await any in-flight maintenance await d.stop() - maintenanceCompleted = true + stopResolved = true - expect(maintenanceCompleted).toBe(true) + // Verify stop() resolved without hanging + expect(stopResolved).toBe(true) + expect(maintenanceStarted).toBe(true) await d.db.destroy() })あるいは、テストの意図がより明確になるようにテスト名とコメントを調整することも検討してください:
it('stop() resolves without hanging when idle maintenance may be in-flight', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/durably/tests/shared/worker.shared.ts` around lines 81 - 110, The test currently sets maintenanceCompleted = true immediately after awaiting d.stop(), which makes the assertion tautological; instead, verify that stop() awaited an in-flight maintenance by setting the flag from the actual maintenance completion callback (e.g., inside whatever handler signals idle maintenance completion such as the worker's maintenance callback or an emitted event like 'worker:error' or a custom 'maintenance:completed' listener) and then await d.stop() before asserting the flag, or alternatively remove the redundant flag/assertion and rename the test to something like "stop() resolves without hanging when idle maintenance may be in-flight" to reflect the intent; update references to maintenanceCompleted and the test name accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/durably/tests/shared/purge.shared.ts`:
- Around line 271-309: The test omits shutting down the Durably instance: add an
explicit await d.stop() before calling d.db.destroy() so the worker is cleanly
stopped and any in-flight/idle maintenance is awaited; reference the Durably
instance methods processUntilIdle, stop, and the existing d.db.destroy() call
and insert the await d.stop() immediately prior to d.db.destroy().
In `@packages/durably/tests/shared/worker.shared.ts`:
- Around line 94-96: The test registers an unused event listener
d.on('run:leased') which never fires; remove this noop listener or replace it
with a meaningful one that matches the test intent (for example
d.on('worker:error') to assert maintenance or error conditions). Locate the
registration of d.on('run:leased') in the test (symbol: d.on) and either delete
that line or change the event name and accompanying assertion to watch for the
appropriate event (e.g., 'worker:error') so the test actually observes and
asserts on emitted events.
- Around line 81-110: The test currently sets maintenanceCompleted = true
immediately after awaiting d.stop(), which makes the assertion tautological;
instead, verify that stop() awaited an in-flight maintenance by setting the flag
from the actual maintenance completion callback (e.g., inside whatever handler
signals idle maintenance completion such as the worker's maintenance callback or
an emitted event like 'worker:error' or a custom 'maintenance:completed'
listener) and then await d.stop() before asserting the flag, or alternatively
remove the redundant flag/assertion and rename the test to something like
"stop() resolves without hanging when idle maintenance may be in-flight" to
reflect the intent; update references to maintenanceCompleted and the test name
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b340f34-c3d8-4221-b118-791549ead840
📒 Files selected for processing (2)
packages/durably/tests/shared/purge.shared.tspackages/durably/tests/shared/worker.shared.ts
Summary
processOne()to pure claim → execute → return, removingreleaseExpiredLeasesand fire-and-forget auto-purge that broke the "false = idle" contractrunIdleMaintenance, called via workeronIdlecallback andprocessUntilIdle(only when actually idle, not whenmaxRunshit)stop()awaits in-flight idle maintenance and errors in maintenance can't hang the worker loopTest plan
pnpm validate— 28/28 tasks pass (format, lint, typecheck, 297 tests)pnpm --filter @coji/durably test:node:postgres— 11/11 passprocessUntilIdle(awaited) instead ofprocessOne+setTimeouthackprocessOnereturns false with zero background workCloses #122
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
新機能
改善
テスト